program: safe deserialize config keys #16
Merged
+152
−21
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joncinque
reviewed
Oct 31, 2024
Contributor
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
The change looks good! Just a few little things
Contributor
There was a problem hiding this comment.
Is the extra check that expensive? Why are all the CU numbers so much higher? Or is it a new toolchain?
Contributor
Author
There was a problem hiding this comment.
I'm not sure why CUs changed so much, or why the change isn't constant, but I've built this branch with CLI v2.0.2 as declared in the workspace manifest. I cannot guarantee I've always built with v2.0.2 though...
This was referenced Nov 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Working with the Firedancer conformance suite revealed a relatively inconsequential control flow mismatch between the BPF version of the Config program and its original builtin version.
The
limited_deserializefunction is designed to stop deserialization at some input buffer length cap, but it does not limit memory allocations as a result of some deserialized vector length. In the case ofConfigKeys, a very largeShortU16length value could be provided to the program's input, forcing the deserialization step to allocate space for a massive vector of(Pubkey, bool), exhausting the BPF program's heap.The program would still fail, but the error would be different between the builtin and BPF versions. To ensure maximum backwards compatibility, and to give developers a more proper error code, the BPF program's deserialization needs an additional check for allocation size.
Summary of Changes
Add a check for the provided
ShortU16vector length to the program's input deserialization, to catch any would-be large vector allocations before causing an OOM.